Skip to content

fix: wrap vault backup initial read in try/catch to survive Android Keystore key invalidation#29752

Merged
tommasini merged 4 commits into
mainfrom
fix/backup-vault-try-catch
May 6, 2026
Merged

fix: wrap vault backup initial read in try/catch to survive Android Keystore key invalidation#29752
tommasini merged 4 commits into
mainfrom
fix/backup-vault-try-catch

Conversation

@tommasini
Copy link
Copy Markdown
Contributor

@tommasini tommasini commented May 5, 2026


Description

Reason for the change:

On Android, when a user changes their biometric enrollment (or due to an Android 16 Keystore behavioral change), the Keystore key backing the vault backup entry can be permanently invalidated. In that state, getInternetCredentials(VAULT_BACKUP_KEY) throws an exception rather than returning false/undefined. Because that call sat inside the outer try/catch in backupVault, the thrown error was caught at the top-level catch, which logged "Vault backup failed" to Sentry and returned { success: false } — even though the underlying vault was fine and a fresh backup could have been written successfully.

Solution:

Wrap the initial getInternetCredentials read in its own inner try/catch. If the read throws (key invalidated), we:

  1. Log a non-error warning via Logger.log (no Sentry noise)
  2. Clear any corrupted keychain entries with clearAllVaultBackups()
  3. Fall through to write a fresh backup normally

This makes backupVault resilient to stale Android Keystore state and prevents a false "Vault backup failed" from ever surfacing when the real backup operation can succeed.

Changelog

CHANGELOG entry: Fixed an issue on Android where vault backup could silently fail after a biometric enrollment change or Keystore key invalidation

Related issues

No issue: bug surfaced via Sentry report (extra.message: "Vault backup failed") — Android Keystore key invalidation causing getInternetCredentials to throw inside backupVault

Manual testing steps

Feature: Vault backup resilience on Android

  Scenario: Vault backup succeeds even when existing backup read throws due to Keystore invalidation
    Given the user has a MetaMask wallet set up on Android
    And a vault backup already exists in the keychain
    And the Android Keystore key has been invalidated (e.g. by adding/removing biometric enrollment)

    When the vault backup routine is triggered (e.g. on wallet unlock or state change)
    Then the backup should complete successfully
    And no "Vault backup failed" error should appear in Sentry
    And the user should not be prompted with an error or crash

Note: This fix is primarily defensive for an Android Keystore edge case. Manual reproduction requires a device where biometrics have been changed after the backup was created, or an Android 16 device exhibiting the Keystore regression. Unit test coverage added for the throwing path.

Screenshots/Recordings

Before

N/A

After

N/A

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Made with Cursor


Note

Low Risk
Low risk, localized change that only alters error handling around reading existing keychain credentials and adds a unit test for the throwing path. Main risk is potentially masking unexpected read failures by proceeding to overwrite the backup.

Overview
Makes backupVault resilient to Android Keystore/keychain read exceptions by wrapping the initial getInternetCredentials(VAULT_BACKUP_KEY) call in an inner try/catch and proceeding as if no existing backup was found (while logging via Logger.log).

Adds a unit test ensuring vault backup still reports success when the existing-backup read throws (simulating Android Keystore key invalidation).

Reviewed by Cursor Bugbot for commit 6a6edf9. Bugbot is set up for automated code reviews on this repo. Configure here.

@tommasini tommasini self-assigned this May 5, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@github-actions github-actions Bot added the pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. label May 5, 2026
@metamaskbotv2 metamaskbotv2 Bot added the team-mobile-platform Mobile Platform team label May 5, 2026
@github-project-automation github-project-automation Bot moved this to Needs dev review in PR review queue May 5, 2026
@github-actions github-actions Bot added the size-S label May 5, 2026
Copy link
Copy Markdown
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment

Comment thread app/core/BackupVault/backupVault.ts Outdated
Copy link
Copy Markdown
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-project-automation github-project-automation Bot moved this from Needs dev review to Review finalised - Ready to be merged in PR review queue May 5, 2026
@tommasini tommasini marked this pull request as ready for review May 5, 2026 17:41
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 43ff1dd. Configure here.

expect(response).toEqual(mockedSuccessResponse);
});

it('should still succeed if reading the existing backup throws (e.g. Android Keystore key invalidation)', async () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test name uses banned "should" prefix

Low Severity

The new test name 'should still succeed if reading the existing backup throws...' uses the word "should", which is banned per the unit testing guidelines ("NEVER use 'should' in test names — this is a hard rule with zero exceptions"). An action-oriented alternative like 'completes backup when reading existing backup throws due to Keystore key invalidation' would conform to the rule.

Fix in Cursor Fix in Web

Triggered by project rule: Unit Testing Guidelines

Reviewed by Cursor Bugbot for commit 43ff1dd. Configure here.

@tommasini tommasini added the no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed label May 5, 2026
@tommasini tommasini enabled auto-merge May 5, 2026 22:41
@tommasini tommasini removed the pr-not-ready-for-e2e Skip E2E and block merging. Remove this label once the PR is ready to run the E2E tests. label May 5, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 5, 2026

🔍 Smart E2E Test Selection

  • Selected E2E tags: None (no tests recommended)
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: low
  • AI Confidence: 90%
click to see 🤖 AI reasoning details

E2E Test Selection:
The changes are a targeted defensive bug fix in app/core/BackupVault/backupVault.ts:

  1. What changed: The backupVault() function now wraps the getInternetCredentials(VAULT_BACKUP_KEY) call in its own try/catch. This handles Android Keystore key invalidation scenarios (biometric enrollment changes, Android 16 behavioral changes) where the API throws instead of returning false/undefined. On error, it logs and treats the situation as "no existing backup," allowing the fresh backup write to proceed.

  2. Scope: Purely a defensive error-handling addition. The happy path is completely unchanged. No UI, navigation, controller state, or shared component changes.

  3. Test coverage: A new unit test was added in backupVault.test.ts that directly validates the new behavior by mocking getInternetCredentials to throw and asserting the backup still succeeds.

  4. E2E relevance: No E2E test tag covers Android Keystore invalidation scenarios — this is a device-level edge case that unit tests are the appropriate vehicle for. The change doesn't affect any flows tested by the available E2E tags (confirmations, swaps, accounts, identity, network, browser, snaps, etc.).

  5. Risk: Low. The fix is additive (adds error resilience), doesn't change behavior on the success path, and is well-covered by the new unit test.

No E2E tags are warranted for this change.

Performance Test Selection:
The change is a defensive error-handling fix in the vault backup read path. It adds a try/catch around a single keychain read operation. There is no UI rendering, no list components, no state management changes, and no impact on app startup or critical user flows that would affect performance metrics.

View GitHub Actions results

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 6, 2026

@tommasini tommasini added this pull request to the merge queue May 6, 2026
Merged via the queue into main with commit d8db24a May 6, 2026
100 of 103 checks passed
@tommasini tommasini deleted the fix/backup-vault-try-catch branch May 6, 2026 00:17
@github-actions github-actions Bot locked and limited conversation to collaborators May 6, 2026
@metamaskbotv2 metamaskbotv2 Bot added the release-7.77.0 Issue or pull request that will be included in release 7.77.0 label May 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed release-7.77.0 Issue or pull request that will be included in release 7.77.0 size-S team-mobile-platform Mobile Platform team

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants